Fix reinstall not detecting changed permissions by itself by MetRonnie · Pull Request #6658 · cylc/cylc-flow

MetRonnie · 2025-03-06T16:32:21Z

This fixes a bug where cylc reinstall in interactive mode would not pick up on permissions changes for files.

The reason for this is that rsync does not report files as included to be sent if only the permissions have been changed, unless %i is added to the --out-format option. %i is an itemized changes summary.

https://linux.die.net/man/1/rsync, https://man.freebsd.org/cgi/man.cgi?query=rsync

The "%i" escape has a cryptic output that is 11 letters long. The general format is like the string YXcstpoguax, where Y is replaced by the type of update being done, X is replaced by the file-type, and the other letters represent attributes that may be output if they are being modified.

I have hidden this itemized changes summary unless using cylc reinstall -v or even higher verbosity.

Check List

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
No dependency changes
Tests are included
Changelog entry included if this is a change that can affect users
No docs needed
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

MetRonnie · 2025-03-10T13:59:23Z

cylc/flow/install.py

-    stdout, stderr = proc.communicate()
-
-    # Strip unwanted output.
-    stdout = ('\n'.join(re.findall(r'\+\+\+(.*)\+\+\+', stdout))).strip()


This seemed to be duplicating the work of format_rsync_output(), but removing all cannot delete non-empty directory: lines, not just the opt dir

The point of the +++ prefix/suffix will have been to separate intended rsync output from any unexpected "junk" that might be output by the command / shell, ensuring that we only display what we need, even if the output differs from expectation in other ways.

Why did you remove this? Just didn't like the look of it?

Remember that this code is used for more than just the dry-run check.

From the context I could gather in #5125, the only unwanted output that was being targeted was cannot delete non-empty directory:. This is implemented in format_reinstall_output()

This rsync command doesn't just apply to the dry-run check (which is passed throughformat_rsync_output). And this prefix stripped out other potential output.

cylc-flow/cylc/flow/install.py

Lines 226 to 227 in 48d3d38

# Strip unwanted output.

stdout = ('\n'.join(re.findall(r'\+\+\+(.*)\+\+\+', stdout))).strip()

is at odds with

cylc-flow/cylc/flow/scripts/reinstall.py

Lines 346 to 348 in 48d3d38

else:

# other uncategorised log line

lines.append(line)

The former seems to have been implemented to cover cannot delete non-empty directory:, there was no mention of any other potential rsync output in #5125. And the latter seems to have been implemented to deliberately include other potential rsync output in case the rsync implementation results in non-standard output format.

Hence I am not sure we should be stripping other potential rsync output.

It's not at odds.

The +++ prefix ensures that only the intended output lines get through.

The else handles intended output lines that don't match send or del..

cannot delete non-empty directory: does not have +++ pre/suffixed to it (it is orthogonal to the %o %n%L output format). It is always being stripped. Only lines that match send or del. have +++ pre/suffixed anyway.

The only case I can think of where some other potential output would make it through both bits of code is if the %o output format resulted in something other than send or del.. But that seems unlikely

wxtim · 2025-03-11T08:34:47Z

cylc/flow/scripts/reinstall.py

+            lines.append(cparse(formatted_line))
        else:
-            # other uncategorised log line
+            # shouldn't happen; tolerate unknown rsync implementation?


If you don't think it should happen, then perhaps a message warning that this is an unexpected rsync implementation, please contact the dev team might be appropriate.

There are multiple potential causes of this:

Alternative rsync implementation (e.g. BSD rsync).

Newer/older version of GNU rsync

Other misc stdout created by ssh or bash.

We just need to keep our scripts as robust to output changes as possible. No need to bother the user about it.

cylc/flow/scripts/reinstall.py

wxtim

Tested manually
Read
Made 2 non-blocking suggestions

oliver-sanders · 2025-03-11T10:50:00Z

cylc/flow/remote.py

    '-a',
    '--checksum',
-    '--out-format=%o %n%L',
+    '--out-format=%i %o %n%L',  # see comment below


Do we want to include %i in the logs?

If this is only really relevant to the dry-run functionality, then continue to override the CLI argument as we did before (no need to change here).

If we think we should have this in the logs, then to keep them readable it might be better to keep the operation at the start of the line?

The rsync output isn't included in the reinstall logs. The only thing we do with the rsync output is print it to stdout in the dry-run case.

I think it is, install a workflow, make changes, then check ~/cylc-run/<workflow>/log/install/*.

Ah ok - it's only included in the 01-install.log. So I should make this option with %i only apply to reinstall. But it is not included in the reinstall logs regardless.

Will tackle in follow-up.

wxtim

Checked changes since last review
Deconflicted
Lint check failures unrelated

MetRonnie added bug Something is wrong :( small labels Mar 6, 2025

MetRonnie added this to the 8.4.2 milestone Mar 6, 2025

MetRonnie self-assigned this Mar 6, 2025

MetRonnie force-pushed the rsync branch 2 times, most recently from b4197cd to f8e206e Compare March 10, 2025 13:46

MetRonnie requested review from oliver-sanders and wxtim March 10, 2025 13:51

MetRonnie marked this pull request as ready for review March 10, 2025 13:52

MetRonnie commented Mar 10, 2025

View reviewed changes

wxtim reviewed Mar 11, 2025

View reviewed changes

cylc/flow/scripts/reinstall.py Show resolved Hide resolved

wxtim approved these changes Mar 11, 2025

View reviewed changes

oliver-sanders reviewed Mar 11, 2025

View reviewed changes

MetRonnie added 4 commits March 11, 2025 15:20

Fix reinstall not detecting changed permissions by itself

03e8bdb

cylc reinstall: print rsync stdout in stdout not stderr

9db9521

Add test for cylc reinstall with permissions changed

50421d7

Changelog

144600b

MetRonnie force-pushed the rsync branch from f8e206e to 144600b Compare March 12, 2025 17:35

Merge branch '8.4.x' into rsync

3f37872

wxtim approved these changes Apr 7, 2025

View reviewed changes

oliver-sanders approved these changes Apr 7, 2025

View reviewed changes

oliver-sanders merged commit 59f3da4 into cylc:8.4.x Apr 7, 2025
27 of 28 checks passed

oliver-sanders mentioned this pull request Apr 7, 2025

reinstall: log reinstallation in the reinstall log #6710

Merged

8 tasks

MetRonnie deleted the rsync branch April 7, 2025 13:48

	# Strip unwanted output.
	stdout = ('\n'.join(re.findall(r'\+\+\+(.*)\+\+\+', stdout))).strip()

Conversation

MetRonnie commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliver-sanders Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetRonnie Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliver-sanders Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetRonnie Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetRonnie Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wxtim left a comment

Choose a reason for hiding this comment

Uh oh!

oliver-sanders Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetRonnie Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxtim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MetRonnie commented Mar 6, 2025 •

edited

Loading

oliver-sanders Mar 11, 2025 •

edited

Loading

MetRonnie Mar 11, 2025 •

edited

Loading

oliver-sanders Mar 11, 2025 •

edited

Loading

MetRonnie Mar 11, 2025 •

edited

Loading

MetRonnie Mar 11, 2025 •

edited

Loading

oliver-sanders Mar 11, 2025 •

edited

Loading

MetRonnie Mar 11, 2025 •

edited

Loading